[ES-2790] Compute Public Key Hash & PATCH Client Management API#1618
[ES-2790] Compute Public Key Hash & PATCH Client Management API#1618ase-101 merged 20 commits intomosip:developfrom
Conversation
Signed-off-by: Md-Humair-KK <mdhumair.kankudti@gmail.com>
Signed-off-by: Md-Humair-KK <mdhumair.kankudti@gmail.com>
Signed-off-by: Md-Humair-KK <mdhumair.kankudti@gmail.com>
Signed-off-by: Md-Humair-KK <mdhumair.kankudti@gmail.com>
Signed-off-by: Md-Humair-KK <mdhumair.kankudti@gmail.com>
Signed-off-by: Md-Humair-KK <mdhumair.kankudti@gmail.com>
Signed-off-by: Md-Humair-KK <mdhumair.kankudti@gmail.com>
WalkthroughAdds a PATCH API and end-to-end flow to partially update OIDC client records: new patch DTO and SPI method, controller endpoint, service methods (buildClient + patchClient) with encPublicKey JWK handling and hashing, persistence, cache eviction, auditing, OpenAPI docs, tests, and Postman updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller
participant Service
participant IdentityUtil as IdentityProviderUtil
participant Repo as ClientDetailRepository
participant Cache
participant Audit
Client->>Controller: PATCH /client-mgmt/client/{client_id} + patchRequest
Controller->>Service: patchClient(clientId, patchRequest)
Service->>Repo: findByClientId(clientId)
Repo-->>Service: existing ClientDetail
alt encPublicKey provided
Service->>IdentityUtil: validate/serialize encPublicKey JWK
IdentityUtil-->>Service: encPublicKeyJson + encPublicKeyHash
end
Service->>Service: buildClient(existing, patchRequest) (merge fields, timestamps)
Service->>Repo: save(updated ClientDetail)
Repo-->>Service: saved ClientDetail
Service->>Audit: record audit entry (OAUTH_CLIENT_PATCH)
Service->>Cache: evict(clientId)
Service-->>Controller: ClientDetailResponse
Controller-->>Client: 200 OK + response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In
`@client-management-service-impl/src/main/java/io/mosip/esignet/services/ClientManagementServiceImpl.java`:
- Around line 328-412: The patch loses the original client name when
objectMapper.readValue fails: capture the raw existingName before parsing in
buildClient, and if parsing throws, populate existingNameMap with a single entry
mapping Constants.NONE_LANG_KEY to the original existingName (if non-null)
instead of leaving it empty; then proceed to derive clientName and
clientNameLangMap and call getClientNameLanguageMapAsJsonString as before so the
original name is preserved when only clientNameLangMap is provided.
In `@db_scripts/mosip_esignet/ddl/esignet-client_detail.sql`:
- Around line 28-29: The enc_public_key column is defined as varchar(1024) here
but other schemas use varchar(2048), which risks truncation; update the DDL
(column enc_public_key) and any related upgrade/migration scripts and test
schema definitions to use varchar(2048) consistently, and verify any indexes,
length checks, or hashing logic that reference enc_public_key length (e.g.,
enc_public_key_hash generation) are adjusted accordingly to avoid mismatches.
In `@db_upgrade_script/mosip_esignet/sql/1.7.1_to_1.8.0_upgrade.sql`:
- Around line 105-107: The ALTER TABLE in the upgrade script adds enc_public_key
as varchar(1024) which is smaller than the target/test schema; update the
migration to use the intended length (change enc_public_key to varchar(2048)) so
it matches the target DDL and tests, and ensure any related DDL/tests
referencing enc_public_key size are updated to 2048 as well; keep
enc_public_key_hash as-is unless tests require a different length.
In `@docs/esignet-openapi.yaml`:
- Around line 1453-1456: The PATCH schema currently omits validation constraints
(enums, uniqueItems, minItems, date-time format) for fields like requestTime and
request; update the PATCH schema to reuse the same property constraints as the
PUT/POST schemas so supplied fields are validated identically — either $ref the
existing PUT/POST property schemas or copy the exact constraints (including enum
lists, uniqueItems/minItems, and format: date-time) into PATCH properties (or
use allOf with the PUT/POST schema) so any provided field must meet the same
rules; apply the same change for the other affected schema block referenced in
the comment.
🧹 Nitpick comments (6)
client-management-service-impl/src/test/java/io/mosip/esignet/ClientDetailRepositoryTest.java (1)
293-319: Assert encPublicKeyHash values to fully validate persistence.
Right now only null/non-null is verified. Capture expected hashes and assert equality after fetch/update.🧪 Suggested test tightening
@@ - clientDetail.setEncPublicKeyHash(UUID.randomUUID().toString()); + String encPublicKeyHash = UUID.randomUUID().toString(); + clientDetail.setEncPublicKeyHash(encPublicKeyHash); @@ - Assertions.assertEquals("DUMMY ENC PUBLIC KEY", result.get().getEncPublicKey()); + Assertions.assertEquals("DUMMY ENC PUBLIC KEY", result.get().getEncPublicKey()); + Assertions.assertEquals(encPublicKeyHash, result.get().getEncPublicKeyHash()); @@ - clientDetail.setEncPublicKeyHash(UUID.randomUUID().toString()); + String updatedEncPublicKeyHash = UUID.randomUUID().toString(); + clientDetail.setEncPublicKeyHash(updatedEncPublicKeyHash); @@ - Assertions.assertEquals("UPDATED ENC PUBLIC KEY", result.get().getEncPublicKey()); + Assertions.assertEquals("UPDATED ENC PUBLIC KEY", result.get().getEncPublicKey()); + Assertions.assertEquals(updatedEncPublicKeyHash, result.get().getEncPublicKeyHash());Also applies to: 347-378
postman-collection/eSignet.postman_collection.json (2)
277-280: Remove duplicate alg/use assignments to reduce confusion.♻️ Suggested cleanup
- "// Add alg and use values to client public key", - "publicKey_jwk.alg = \"RS256\";", - "publicKey_jwk.use = \"sig\";", - "",
379-445: PATCH item is useful; consider normalizingencPublicKeyeven when reused.
Ifencryption_public_keyalready exists but lacksalg/use, the PATCH call can fail validation. Reasserting these fields each time keeps runs deterministic.🛠️ Suggested tweak
- "const existingEncPublicKey = pm.environment.get(\"encryption_public_key\");", - "if (!existingEncPublicKey || existingEncPublicKey === '' || existingEncPublicKey === 'null' || existingEncPublicKey === '{}') {", + "const existingEncPublicKey = pm.environment.get(\"encryption_public_key\");", + "let enc_publicKey_jwk;", + "if (!existingEncPublicKey || existingEncPublicKey === '' || existingEncPublicKey === 'null' || existingEncPublicKey === '{}') {", " enc_kp = pmlib.rs.KEYUTIL.generateKeypair(\"RSA\", 2048);", " enc_privateKey_jwk = pmlib.rs.KEYUTIL.getJWK(enc_kp.prvKeyObj);", " enc_publicKey_jwk = pmlib.rs.KEYUTIL.getJWK(enc_kp.pubKeyObj);", @@ - " pm.environment.set(\"encryption_private_key\", JSON.stringify(enc_privateKey_jwk));", - " pm.environment.set(\"encryption_public_key\", JSON.stringify(enc_publicKey_jwk));", - "}", + " pm.environment.set(\"encryption_private_key\", JSON.stringify(enc_privateKey_jwk));", + "} else {", + " enc_publicKey_jwk = JSON.parse(existingEncPublicKey);", + "}", + "enc_publicKey_jwk.alg = \"RSA-OAEP-256\";", + "enc_publicKey_jwk.use = \"enc\";", + "pm.environment.set(\"encryption_public_key\", JSON.stringify(enc_publicKey_jwk));",client-management-service-impl/src/test/java/io/mosip/esignet/ClientManagementServiceTest.java (3)
343-364: Consider using ArgumentCaptor to verify patched entity state.The test verifies the response but doesn't confirm that the
ClientDetailentity passed tosave()has the expected patched values. UsingArgumentCaptorwould strengthen this test.♻️ Suggested enhancement
+ `@Captor` + ArgumentCaptor<ClientDetail> clientDetailCaptor; + `@Test` public void patchClient_withValidClientId_thenPass() throws EsignetException { ClientDetail clientDetail = createMockClientDetail("client_id_v1"); Mockito.when(clientDetailRepository.findById("client_id_v1")).thenReturn(Optional.of(clientDetail)); ClientDetailPatchRequest patchRequest = new ClientDetailPatchRequest(); patchRequest.setClientName("updated_client_name"); patchRequest.setLogoUri("http://service.com/new_logo.png"); patchRequest.setRedirectUris(Arrays.asList("http://service.com/callback")); patchRequest.setStatus("ACTIVE"); ClientDetail savedEntity = new ClientDetail(); savedEntity.setId("client_id_v1"); savedEntity.setStatus("ACTIVE"); - Mockito.when(clientDetailRepository.save(Mockito.any(ClientDetail.class))).thenReturn(savedEntity); + Mockito.when(clientDetailRepository.save(clientDetailCaptor.capture())).thenReturn(savedEntity); ClientDetailResponse response = clientManagementService.patchClient("client_id_v1", patchRequest); Assertions.assertNotNull(response); Assertions.assertEquals("client_id_v1", response.getClientId()); Assertions.assertEquals("ACTIVE", response.getStatus()); + + ClientDetail captured = clientDetailCaptor.getValue(); + Assertions.assertEquals("http://service.com/new_logo.png", captured.getLogoUri()); }
366-379: Consider usingassertThrowsfor exception testing (optional).While the try-catch pattern is consistent with existing tests in this file, JUnit 5's
assertThrowsprovides cleaner assertion syntax and ensures the test fails if the exception is not thrown.♻️ Modern JUnit 5 approach
`@Test` public void patchClient_withNonExistingClientId_thenFail() { Mockito.when(clientDetailRepository.findById("non_existing_client")).thenReturn(Optional.empty()); ClientDetailPatchRequest patchRequest = new ClientDetailPatchRequest(); patchRequest.setClientName("updated_name"); - try { - clientManagementService.patchClient("non_existing_client", patchRequest); - Assertions.fail(); - } catch (EsignetException ex) { - Assertions.assertEquals(ErrorConstants.INVALID_CLIENT_ID, ex.getErrorCode()); - } + EsignetException ex = Assertions.assertThrows(EsignetException.class, + () -> clientManagementService.patchClient("non_existing_client", patchRequest)); + Assertions.assertEquals(ErrorConstants.INVALID_CLIENT_ID, ex.getErrorCode()); }
701-718: Consider verifying that the timestamp is actually updated.The comment states "only timestamp should be updated," but the test doesn't verify this. Using
ArgumentCaptorto check thatupdatedTimesis set (and is recent) would strengthen this test.♻️ Suggested verification
`@Test` public void patchClient_withEmptyPatchRequest_thenOnlyUpdateTimestamp() throws EsignetException { ClientDetail clientDetail = createMockClientDetail("client_id_v1"); + LocalDateTime originalTimestamp = LocalDateTime.now().minusDays(1); + clientDetail.setUpdatedTimes(originalTimestamp); Mockito.when(clientDetailRepository.findById("client_id_v1")).thenReturn(Optional.of(clientDetail)); ClientDetailPatchRequest patchRequest = new ClientDetailPatchRequest(); // All fields are null - only timestamp should be updated ClientDetail savedEntity = new ClientDetail(); savedEntity.setId("client_id_v1"); savedEntity.setStatus("ACTIVE"); - Mockito.when(clientDetailRepository.save(Mockito.any(ClientDetail.class))).thenReturn(savedEntity); + ArgumentCaptor<ClientDetail> captor = ArgumentCaptor.forClass(ClientDetail.class); + Mockito.when(clientDetailRepository.save(captor.capture())).thenReturn(savedEntity); ClientDetailResponse response = clientManagementService.patchClient("client_id_v1", patchRequest); Assertions.assertNotNull(response); Assertions.assertEquals("client_id_v1", response.getClientId()); + Assertions.assertTrue(captor.getValue().getUpdatedTimes().isAfter(originalTimestamp)); }
...gement-service-impl/src/main/java/io/mosip/esignet/services/ClientManagementServiceImpl.java
Show resolved
Hide resolved
Signed-off-by: Md-Humair-KK <mdhumair.kankudti@gmail.com>
Signed-off-by: Md-Humair-KK <mdhumair.kankudti@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@docs/esignet-openapi.yaml`:
- Around line 1559-1588: The encPublicKey schema currently allows kty: EC but
doesn't define EC-specific properties (crv, x, y) and only requires kty, letting
incomplete JWKs pass; update the schema referenced by encPublicKey to either (A)
restrict the kty enum to only "RSA" and keep required RSA props (n, e), or (B)
replace the flat schema with a discriminated oneOf/oneOf-like conditional on kty
that defines an RSA variant (required: kty="RSA", n, e; alg/use/kid as now) and
an EC variant (required: kty="EC", crv, x, y; plus alg/use/kid as appropriate).
Ensure you reference and adjust kty, n, e, crv, x, y and required arrays so
incomplete JWKs are rejected.
- Around line 1548-1549: PATCH operation's request schema currently allows any
object for additionalConfig; update the PATCH request body schema's
additionalConfig definition to mirror the explicit properties used by PUT/POST
(same property names, types, required flags and validations) so the contract is
consistent and the API can return the documented invalid_additional_config
error; locate the PATCH operation's requestBody/schema that references
additionalConfig and replace the loose "type: object" with the same object
schema (or a $ref to the PUT/POST additionalConfig schema) and ensure any
examples/validation keywords match.
🧹 Nitpick comments (1)
docs/esignet-openapi.yaml (1)
1551-1553: Replacenullable: truewith JSON Schema's type array syntax in OpenAPI 3.1.
OpenAPI 3.1 aligns with JSON Schema and removes thenullablekeyword entirely. Usetype: [object, "null"]instead.🔧 Suggested fix
encPublicKey: - type: object - nullable: true + type: + - object + - 'null'
Signed-off-by: Md-Humair-KK <mdhumair.kankudti@gmail.com>
Signed-off-by: Md-Humair-KK <mdhumair.kankudti@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/esignet-openapi.yaml`:
- Around line 1473-1477: The PATCH `status` enum currently uses uppercase values
"ACTIVE"/"INACTIVE" which conflict with the PUT API that uses lowercase
"active"/"inactive"; update the enum for the PATCH `status` field to use
"active" and "inactive" (replace "ACTIVE"/"INACTIVE") so client docs match the
existing update APIs and backend expectations, locating the `status` property in
the OpenAPI schema where the enum is defined and making the value changes.
🧹 Nitpick comments (1)
docs/esignet-openapi.yaml (1)
1427-1430: Align description label withencPublicKeyfield name.Line 1427 refers to
enc_public_key, but the field isencPublicKey. This can confuse API consumers.✏️ Suggested doc tweak
- **Special handling for enc_public_key:** + **Special handling for encPublicKey:**
Signed-off-by: Md-Humair-KK <mdhumair.kankudti@gmail.com>
esignet-core/src/main/java/io/mosip/esignet/core/dto/ClientDetailPatchRequest.java
Show resolved
Hide resolved
Signed-off-by: Md-Humair-KK <mdhumair.kankudti@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@esignet-core/src/main/java/io/mosip/esignet/core/dto/ClientDetailPatchRequest.java`:
- Line 61: Add DTO-level validation for the encPublicKey field in
ClientDetailPatchRequest by creating a custom constraint annotation (e.g.,
`@ValidJWK`) and a corresponding ConstraintValidator that validates the JWK map
structure (ensure required RSA fields kty, n, e or EC fields crv, x, y, and
valid types/values). Annotate the encPublicKey field in ClientDetailPatchRequest
with `@ValidJWK` and configure the validator to produce clear violation messages;
wire the annotation with javax.validation (or jakarta.validation) so
controller-level validation triggers and fails fast before service-layer calls
like IdentityProviderUtil.getJWKString() and computePublicKeyHash().
🧹 Nitpick comments (1)
esignet-core/src/main/java/io/mosip/esignet/core/dto/ClientDetailPatchRequest.java (1)
55-56: Consider adding a size constraint onclientNameLangMap.While keys and values have validation, the map itself has no size limit. An unbounded map could accept excessive language entries, potentially causing memory pressure.
Other collection fields have explicit
@Sizeconstraints (e.g.,redirectUrismax=5,userClaimsmax=30). For consistency and defensive coding, consider adding a similar constraint:Proposed fix
- private Map<@ClientNameLang String, + `@Size`(max = 20, message = ErrorConstants.INVALID_CLIENT_NAME_LANG_MAP) + private Map<@ClientNameLang String, `@Size`(max = 50, message = ErrorConstants.INVALID_CLIENT_NAME_LENGTH) String> clientNameLangMap;
esignet-core/src/main/java/io/mosip/esignet/core/dto/ClientDetailPatchRequest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/esignet-openapi.yaml`:
- Around line 1575-1584: The encPublicKey schema uses the removed OpenAPI 3.1
"nullable" flag and currently lists only object variants under oneOf, so null
values will fail validation; update the encPublicKey definition (the oneOf block
for encPublicKey) to include an explicit JSON Schema null branch (e.g., add a
oneOf entry with type: 'null') so the schema accepts null to clear the key while
keeping the existing object variants (like the "RSA Encryption Key" entry)
unchanged.
Signed-off-by: Md-Humair-KK <mdhumair.kankudti@gmail.com>
Signed-off-by: Md-Humair-KK <mdhumair.kankudti@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@client-management-service-impl/src/main/java/io/mosip/esignet/services/ClientManagementServiceImpl.java`:
- Around line 346-373: The code in ClientManagementServiceImpl is mutating the
incoming patchRequest by calling removeAll(NULL) on its lists; instead, for each
nullable list (redirectUris, userClaims, authContextRefs, grantTypes,
clientAuthMethods) create a defensive copy (e.g., new List from
patchRequest.getXxx()), call removeAll(NULL) on the copy, then serialize the
copy to JSON and call clientDetail.setRedirectUris / setClaims / setAcrValues /
setGrantTypes / setClientAuthMethods accordingly; do not modify patchRequest
itself and continue to handle patchRequest.getStatus() the same way. Ensure you
reference the NULL constant when filtering.
🧹 Nitpick comments (1)
client-management-service-impl/src/main/java/io/mosip/esignet/services/ClientManagementServiceImpl.java (1)
401-409: Minor: Add space before exception parameter in log statement.📝 Suggested fix
} catch (EsignetException e) { - log.error("Invalid encryption public key",e); + log.error("Invalid encryption public key", e); throw e; }
...gement-service-impl/src/main/java/io/mosip/esignet/services/ClientManagementServiceImpl.java
Show resolved
Hide resolved
Signed-off-by: Md-Humair-KK <mdhumair.kankudti@gmail.com>
Summary by CodeRabbit
New Features
Documentation
Tests
Chores